The Static Analysis tool Scan-Build added that variable localTime can have garbage content if localtime() function called from MT_safe_localtime fails.

Andi, what should we do with this one ? :)

I think a clarification is needed for this bug. MT_safe_localtime fails only if : >> tmPtr = localtime(clock); >> >>#if defined(WIN16) || defined(XP_OS2) >> if ( (PRInt32) *clock < 0 || >> ( (PRInt32) *clock == 0 && tmPtr->tm_year != 70)) >> result = NULL; >> else >> *result = *tmPtr; >>#else >> if (tmPtr) { >> *result = *tmPtr; >> } else { >> result = NULL; >> } >>#endif /* WIN16 */ localtime returns a null ptr

Because you attached a diff against the firefox source tree, a general reminder: NSPR changes must be commited to the separate NSPR repository, and firefox consumes updates to NSPR as new releases of the NSPR project. https://hg.mozilla.org/projects/nspr

Is memset(0) really the correct fix for this bug? struct tm has a member tm_mday where 0 is an illegal value. Will a value of zero for all members of this struct still achieve that the function works correctly? If you wanted to prevent that incorrect data is used, wouldn't it be best to check if the result of MT_safe_localtime is NULL, which indicates that an error has occurred? After the call, the current implementation assumes that it has obtained the values for Jan 2 1970. I haven't read the code, but I'd assume, the logic implemented in this function will fail, if the exploded values are all zero. The function should be changed to produce a safe result, if the call to localtime has failed, such as assuming a zero offset from GMT, and filling the output result accordingly.

Comment on attachment 8789706 [details] Bug 1227497 - added memset on localtime. https://reviewboard.mozilla.org/r/77840/#review86068 ::: nsprpub/pr/src/misc/prtime.c:612 (Diff revision 1) > > secs = 86400L; > + > + memset(&localTime, 0, sizeof(struct tm)); > + > (void) MT_safe_localtime(&secs, &localTime); The correct fix is to check the return value of MT_safe_localtime().

I see PR_LocalTimeParameters cannot return a failure status. I am afraid that it will need to call abort() if MT_safe_localtime() fails.

Thanks for guiding me on this, will repudiate the patch.

MT_safe_localtime was written almost 20 years ago. I just checked the current status. 1. The current version of Single Unix Specification still says "The localtime() function need not be thread-safe." but we should be able to assume localtime_r() is available on Unix. http://pubs.opengroup.org/onlinepubs/9699919799/functions/localtime.html Right now we use localtime_r() only if the HAVE_POINTER_LOCALTIME_R is defined. nspr/configure.in doesn't have a feature test for localtime_r(). We should add one. Until then, we need to manually define HAVE_POINTER_LOCALTIME_R for each Unix platform. In this patch I added it to Linux and Mac OS X. 2. Visual C++ has had localtime_s() since Visual C++ 2005: https://msdn.microsoft.com/en-us/library/a442x3ye(v=vs.80).aspx In this patch I also added a definition of MT_safe_localtime() based on localtime_s(). Note that localtime() also became thread-safe in Visual C++ 2005: Both the 32-bit and 64-bit versions of gmtime, mktime, mkgmtime, and localtime all use a single tm structure per thread for the conversion. Each call to one of these routines destroys the result of the previous call. https://msdn.microsoft.com/en-us/library/bf12f0hc(v=vs.80).aspx But it's better to use localtime_s().

The previous patch passes the arguments to localtime_s() in the wrong order. This patch corrects that.

Comment on attachment 8802702 [details] [diff] [review] Modernize MT_safe_localtime, v2 Wan-Teh, your explanation makes sense, and the patch looks good to me, thanks for your work, r=kaie

If I understand correctly, Wan-Teh's patch is general improvement for NSPR, but it doesn't implement a fix for this bug. The function might still fail, and if you want to make things work correctly, you must still check if function MT_safe_localtime returns a failure, or not. See comment 8.

(In reply to Wan-Teh Chang from comment #8) > I see PR_LocalTimeParameters cannot return a failure status. > I am afraid that it will need to call abort() if MT_safe_localtime() > fails. Wan-Teh, I'm worried that introducing a new abort() might be too rough. Given that today we might potentially use garbage numbers in the worst case, and nobody has ever complained about this behavior yet, I'm not sure that introducing a hard runtime failure is necessary. If we cannot figure out what localtime is, wouldn't it be sufficient to simply return zero parameters, just like we do for GMT ?

Comment on attachment 8802702 [details] [diff] [review] Modernize MT_safe_localtime, v2 Patch checked in on the NSPR trunk: https://hg.mozilla.org/projects/nspr/rev/1f5630faef26ec49b472006f20dd35bd5957c211 I forgot to include the bug number in the hg commit message. Sorry!

Comment on attachment 8809154 [details] [diff] [review] Check for failing MT_safe_localtime, patch v3 Review of attachment 8809154 [details] [diff] [review]: ----------------------------------------------------------------- Kai: thank you for the patch. You're right. My patch (attachment 8802702 [details] [diff] [review]) is not a fix for this bug. It is just a general improvement. I suggest two changes to your patch. ::: pr/src/misc/prtime.c @@ +605,5 @@ > * since Jan. 2, 1970. > */ > > secs = 86400L; > + if (MT_safe_localtime(&secs, &localTime) == NULL) { It is important to fail here, because this should not happen. We are calling this function with input arguments we control, so it should not fail. If it fails, we need to know and fix our code. Please use this code: if (MT_safe_localtime(&secs, &localTime) == NULL) { PR_Assert("MT_safe_localtime(&secs, &localTime) != NULL", __FILE__, __LINE__); } @@ +1624,5 @@ > + /* We don't return a failure, because past versions of NSPR > + * didn't check for a failure, and might have used random > + * values. To avoid breaking functionality in an NSPR > + * upgrade, let's simply use a zero offset fallback. */ > + zone_offset = 0; PR_ParseTimeStringToExplodedTime returns PRStatus, so we should fail with PR_FAILURE here.

(In reply to Wan-Teh Chang from comment #17) > > > > secs = 86400L; > > + if (MT_safe_localtime(&secs, &localTime) == NULL) { > > It is important to fail here, because this should not happen. > > We are calling this function with input arguments we control, > so it should not fail. If it fails, we need to know and fix > our code. We may not know about all circumstances on all platforms that could cause the function to return a failure, and that were previously hidden. How about only aborting in DEBUG builds, and using the safe fallback in optimized builds? If we abort in debug mode, only, then we can detect errors made by programmers, but will not make it worse if it's impossible to convert to a local timezone in some environments. > @@ +1624,5 @@ > > + /* We don't return a failure, because past versions of NSPR > > + * didn't check for a failure, and might have used random > > + * values. To avoid breaking functionality in an NSPR > > + * upgrade, let's simply use a zero offset fallback. */ > > + zone_offset = 0; > > PR_ParseTimeStringToExplodedTime returns PRStatus, so we should fail with > PR_FAILURE here. Aren't you worried that applications might stop working after they upgrade to a newer NSPR version? If this only happens in some special environments, and if we don't abort, it might be difficult to identify this change as the cause. Since you assume that it won't happen, I suggest we assert/abort in debug builds, and return a failure in optimized builds. That can at least give developers the chance to more easily find this new failure, when they try to debug. I'm attaching a patch that implements my above suggestion.

Comment on attachment 8810819 [details] [diff] [review] Check for failing MT_safe_localtime, patch v4 Review of attachment 8810819 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. Kai, I'm terribly sorry that I forgot to mark this patch reviewed when I reviewed it. I wanted to point out that we should not assume people may run our debug builds. The debug builds of some of the most important NSPR-based products (such as Firefox) are too slow to be practical. This is why I no longer rely on using PR_ASSERT to detect an error. The problem with returning PR_GMTParameters is that it is off by hours, which may cause more serious trouble than aborting the program. This is why I still recommend aborting the program, even in optimized builds. I am no longer an NSPR module owner, so I leave this decision up to you.

Regarding debug builds: - developers at least run debug builds when they test their new code - the firefox automated tests are executed in both optimized and debug builds. If there is a frequent occurrence of the failure situation, it should be noticed, at least as test failures in the automated tests. Regarding your worry that returning PR_GMTParameters from function PR_LocalTimeParameters can cause trouble, because it might be off by several hours: We'll only return that value, if the call to localtime* returns a failure and we're unable to get the local time difference. In this scenario, in the past, the code returned the random contents of an uninitialized value. If this was really happening, it was already much worse. If software is being used despite this incorrect use, being off by a few hours should be less problematic than being off by a random number of days/months/years. My points are: - we're not making it worse - we don't introduce a new crash, but we are handling it leniently, in a less random way then before - we make it possible it detect the failure in debug builds. In my opinion this is an improvement. Thank you for the r+, so I'll check this in. It might be useful to get an additional opinion here, so if someone is reading this bug and has additional thoughts, please speak up.